Skip to content

fix(db): review follow-ups for Cosmian/kms#1224 — deterministic tag-join SQL, metrics option in TOML templates - #3

Closed
Manuthor wants to merge 3 commits into
fix/postgresqlfrom
claude/jolly-volta-85qy24
Closed

fix(db): review follow-ups for Cosmian/kms#1224 — deterministic tag-join SQL, metrics option in TOML templates#3
Manuthor wants to merge 3 commits into
fix/postgresqlfrom
claude/jolly-volta-85qy24

Conversation

@Manuthor

@Manuthor Manuthor commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

Follow-up to Cosmian#1224 after review.

The base branch fix/postgresql in this fork is a copy of the head of Cosmian#1224 (Cosmian:fix/postgresql @ 45be9904), so this diff contains only the review fixes. To merge them into Cosmian#1224, open the upstream PR:
https://github.com/Cosmian/kms/compare/fix/postgresql...Manuthor:kms:claude/jolly-volta-85qy24?expand=1

  • Deterministic Locate-by-tags SQL: select_from_objects emitted one INNER JOIN tags tN per tag while iterating the HashSet returned by Attributes::get_tags. That gave the same search a different SQL text on each call, which wastes SQLite's prepare_cached statement cache and makes query logs hard to compare. Tags are now sorted before the joins are emitted. Added a unit test, tag_joins_are_bound_in_sorted_order.
  • Config sync rule (4.6/4.7): the new metrics_count_interval_secs option was only in resources/kms.toml. It is now also documented, commented out, in crate/server/kms_template.toml and pkg/kms.toml. No wizard step was added, because the similar options (auto_rotation_check_interval_secs, keyset_warn_depth) have none.
  • CHANGELOG/fix_postgresql.md updated accordingly.

Review notes on Cosmian#1224 (no change needed)

  • The per-tag INNER JOIN gives the same results as the old GROUP BY … HAVING COUNT(DISTINCT tag), because each join probes UNIQUE (id, tag), so no row can be repeated.
  • The LIMIT pushdown is disabled when rows are filtered after the query (non-admin HSM visibility), so a page never comes back short.
  • MySQL ? placeholders are bound in the same order as they appear in the SQL text.
  • The PostgreSQL JSON indexes match the query expressions, because the ::jsonb cast on a jsonb column is dropped and the column is converted to jsonb at startup, before the indexes are created. The SQLite RotateAutomatic = 1 partial-index predicate matches BOOL_TRUE_LITERAL.
  • metrics_count_interval_secs = 0 disables the select! arm; max(1) avoids the panic on a zero interval.

Testing

  • cargo test -p cosmian_kms_server_database --lib -- sqlite locate_query: 7/7 passed, including test_db_sqlite, which runs the new find_with_options_test / count_non_destroyed_keys_test and the PRAGMA optimize startup path.
  • cargo clippy -p cosmian_kms_server_database --all-targets -- -D warnings: clean.
  • Pre-commit hooks, including nightly fmt and cargo-deny, pass on the changed files. lychee was skipped with the maintainer's OK: it checks every link in documentation/docs, and some external hosts are unreachable from the sandbox.
  • The PostgreSQL and MySQL tests were not run, because they need live databases.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BEWGdmu3aN8ZgShtDV7etc

@claude
fix(db): deterministic tag-join order in Locate SQL; add metrics_coun…
e68934e 
…t_interval_secs to TOML templates

Sort searched tags before emitting per-tag INNER JOINs so the same search
always yields the same SQL text (keeps SQLite prepare_cached effective and
query logs comparable), and document the new metrics_count_interval_secs
option in the tarball/package TOML templates (server config sync rule).

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01BEWGdmu3aN8ZgShtDV7etc
@Manuthor Manuthor changed the title Migrate build scripts from .github/scripts to .mise/scripts fix(db): review follow-ups for Cosmian/kms#1224 — deterministic tag-join SQL, metrics option in TOML templates Sep 29, 2026
@Manuthor
Manuthor changed the base branch from develop to fix/postgresql September 29, 2026 20:53
@Manuthor Claude

Manuthor commented Sep 29, 2026

Copy link
Copy Markdown
Owner Author

main / test / Test on secret_azure - non-fips failed, but not because of this PR. The job stopped at its first step, before any test ran (log):

[ERROR] Required env var AZURE_TENANT_ID is not set. Azure Key Vault tests require service-principal credentials.

AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET and AZURE_KV_NAME are all empty in this fork, because these repository secrets exist only on Cosmian/kms. This PR only touches the SQL query builder, two TOML templates and a changelog file. No code change can fix this: the fix is to add those secrets to this fork's Actions secrets. The job is already queued again on the current head (f8830f48), and that run will confirm it fails the same way.

Other jobs that need cloud credentials (for example secret_aws, google-cse, gcp-cmek, azure-ekm and XKS) may fail in this fork for the same reason. The jobs that exercise this change pass on f8830f48 so far: clippy, the SQLite builds on Linux, macOS and ARM, and the Windows jobs. The database test jobs (sqlite, psql, mysql, mariadb, percona) are still queued.

Generated by Claude Code

@Manuthor Claude

Manuthor commented Sep 29, 2026

Copy link
Copy Markdown
Owner Author

Three more checks failed, and like secret_azure above, none of them fail because of this PR:

  • Test on secret_aws - non-fips (log): Required env var AWS_ACCESS_KEY_ID is not set. It stopped before any test ran.
  • AWS XKS — remote server (log): KMS_XKS_SSH_PRIVATE_KEY is required. The deploy SSH secrets are empty in this fork.
  • HSM crypt2pay - non-fips (log, current head f8830f48): the PKCS#11 library cannot reach the physical HSM (cannot connect to 193.251.15.196:3001: Connection timed out). No mechanisms are available, so C_GenerateKeyPair returns CKR_MECHANISM_INVALID and test_hsm_crypt2pay_all fails. That test lives in crate/hsm/crypt2pay, which this PR does not touch, and the HSM is only reachable from Cosmian's CI.

No fix exists in code for any of these. They need this fork's Actions secrets and network access to the HSM, or they can simply be ignored in the fork. A re-run would fail the same way, so I am not re-running them. The checks for this change (clippy, SQLite on Linux, macOS and ARM, and Windows) are green. I'll keep watching the database test jobs.

Generated by Claude Code

@Manuthor Claude

Manuthor commented Sep 29, 2026

Copy link
Copy Markdown
Owner Author

Status on f8830f48: all the database test jobs that have finished pass: sqlite - fips, psql - fips, mysql - non-fips, mariadb (both), percona (both), redis and db2. So do the other functional suites. The only failures are jobs that need secrets this fork doesn't have:

  • google-cse (fips and non-fips): Required env var TEST_GOOGLE_OAUTH_CLIENT_ID is not set.
  • secret_aws, secret_azure: same as reported above, now confirmed on this commit.
  • HSM proteccio - non-fips (log): Failed logging in. Return code: 7 (CKR_ARGUMENTS_BAD). .mise/tasks/test/hsm-proteccio passes HSM_USER_PASSWORD="${PROTECCIO_PASSWORD}" and HSM_SLOT_ID="${PROTECCIO_SLOT}". test_all.yml fills those from secrets.PROTECCIO_*, which are empty here, so C_Login gets an empty PIN. crate/hsm/proteccio is untouched by this PR.

None of these can be fixed in code, so I'm not re-running them. Still running: sqlite - non-fips, psql - non-fips, mysql - fips, wasm - non-fips, and softhsm2/utimaco non-fips.

Generated by Claude Code

@Manuthor Manuthor closed this Sep 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Manuthor @claude